-
Notifications
You must be signed in to change notification settings - Fork 468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Quoter update #349
base: main
Are you sure you want to change the base?
Quoter update #349
Conversation
hensha256
commented
Sep 9, 2024
•
edited
Loading
edited
- huge simplification of the return parameters - now we just return the gasEstimate, and the quoted amount in unspecified
- removed the cache, its unneeded as it can just be checked within the _swap function with the parameters available
- refactored various functions into libraries for readability
- updated how the revert is caught to be more accurate with custom errors
/// @return sqrtPriceX96After The sqrt price of the pool after the swap | ||
/// @return initializedTicksLoaded The number of initialized ticks that the swap loaded | ||
/// @return amountIn The input quote for the exactOut swap | ||
/// @return gasEstimate Estimated gas units used for the swap | ||
function quoteExactOutputSingle(QuoteExactSingleParams memory params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to add batched functions so you can get many quotes in one call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they use an external multicall contract for multicalling quoters onchain
on mainnet its here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah cool
|
||
import {ParseBytes} from "@uniswap/v4-core/src/libraries/ParseBytes.sol"; | ||
|
||
library QuoterRevert { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this library a little difficult to reason about because all these functions seem to be doing some of the same things but at various lifecycles of a quote (various parsing, and various reverting) but its unclear when to use what.
some suggestions??
- does
validateRevertReason
need to be its own function? Seems like its just used in this file. Can you collapse it? - for all of the reverting code paths can we decide if we want to use the revert logic in assembly or not?
revertQuote
andbubbleReason
are not consistent with the reverting info. - can you add documentation for whats the difference between those two functions? its unclear.. it seems like
revertQuote
could just usebubbleReason
? - do we need `parseAmountUnspecified as its own function as well? it seems like its just used in parseReturnData? Maybe we can change parseReturnData -> parseAmountUnspecified to be more clear since it doesnt parse any other kind of data?
src/base/BaseV4Quoter.sol
Outdated
// Call this contract with the data in question. Each quote path | ||
(bool success, bytes memory returnData) = address(this).call(data); | ||
if (success) return returnData; | ||
if (returnData.length == 0) revert LockFailure(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand why this is necessarily a lock failure?
assertEq(sqrtPriceX96AfterList[0], 79708304437530892332449657932); | ||
assertEq(initializedTicksLoadedList.length, 1); | ||
assertEq(initializedTicksLoadedList[0], 2); | ||
assertEq(amountIn, 6283); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert gas is > 0? I know it's hard to get accurate on the gas but we know it should at least not be 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done asserts 50k < gas < 400k
function _unlockCallback(bytes calldata data) internal override returns (bytes memory) { | ||
(bool success, bytes memory returnData) = address(this).call(data); | ||
// Every quote path gathers a quote, and then reverts either with QuoteSwap(quoteAmount) or alternative error | ||
if (success) revert UnexpectedCallSuccess(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just starting to look through this but my first thought here.. is I'm wondering if its even possible that this returns true? Like could you write a test that hits this revert?
// reason -> reason+0x1f is the length of the reason string | ||
// reason+0x20 -> reason+0x23 is the selector of QuoteSwap | ||
// reason+0x24 -> reason+0x43 is the amountUnspecified | ||
// reason+0x24 -> reason+0x43 is the quoteAmount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is more of a comment to all of this natspec but this is kinda confusing.
0x20 -> 0x23 is a 3 byte difference?
0x24 -> 0x43 is a 31 byte difference?
Is something not inclusive?
uint160[] memory sqrtPriceX96AfterList, | ||
uint32[] memory initializedTicksLoadedList | ||
); | ||
returns (uint256 amountIn, uint256 gasEstimate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think we should actually return the BalanceDelta, will follow up async w you
|
||
import {ParseBytes} from "@uniswap/v4-core/src/libraries/ParseBytes.sol"; | ||
|
||
library QuoterRevert { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet this looks great!
Looks like gas snapshots just need to be updated |